HDFS-16129. Fixing the signature secret file misusage in HttpFS.#3209
HDFS-16129. Fixing the signature secret file misusage in HttpFS.#3209shuzirra merged 2 commits intoapache:trunkfrom
Conversation
|
💔 -1 overall
This message was automatically generated. |
6a396ce to
63a847c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
@tomicooler The latest build looks way better than before. Could you please check if any of the UT failures are related to your patch? |
63a847c to
47d9b69
Compare
|
💔 -1 overall
This message was automatically generated. |
56a259a to
5f58bbd
Compare
|
💔 -1 overall
This message was automatically generated. |
5f58bbd to
2c49707
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
brumi1024
left a comment
There was a problem hiding this comment.
Thanks @tomicooler for working on this. I had a few minor comments, otherwise looks good to me.
...p-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...p-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...p-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Maybe I'm missing something but this is not doing the same as the old code block.
The old code checked if the configuration object has a key set for the prefix + type.
With the new code, you are just checking if the Stream constructed from authFilterConfigurationPrefixes does not match the prefix + type.
Is this intentional?
There was a problem hiding this comment.
Good catch. it should have been this.conf.get(prefix + "type").equals. Here I find the stream().noneMatch useful with the lambda expression. I won't resolve the thread yet, so I can replace it with a helper function if needed.
There was a problem hiding this comment.
Follow-up question then: How come that this mistake was not caught by any of the tests?
There was a problem hiding this comment.
Probably there is no test for this :/
There was a problem hiding this comment.
I see. You can file a follow up jira for this if you want to :)
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
...p-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
...tpfs/src/test/java/org/apache/hadoop/fs/http/client/TestHttpFSFileSystemLocalFileSystem.java
Outdated
Show resolved
Hide resolved
...op-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
Outdated
Show resolved
Hide resolved
...op-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
Outdated
Show resolved
Hide resolved
...op-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
Outdated
Show resolved
Hide resolved
...op-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
Outdated
Show resolved
Hide resolved
2c49707 to
e228b41
Compare
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
The signature secret file was not used in HttpFs. - if the configuration did not contain the deprecated httpfs.authentication.signature.secret.file option then it used the random secret provider - if both option (httpfs. and hadoop.http.) was set then the HttpFSAuthenticationFilter could not read the file because the file path was not substituted properly !NOTE! behavioral change: the deprecated httpfs. configuration values are overwritten with the hadoop.http. values. The commit also contains a follow up change to the YARN-10814, empty secret files will result in a random secret provider. Change-Id: I5774e1daa1df48436d677aad9e993e997c4c807b
7c42bad to
d2b5b76
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@tomicooler thanks for the updates, it looks good to me, +1 (non-binding). |
Change-Id: I9d9351c9e6b78974825f382d0f18caa60f820a69
|
🎊 +1 overall
This message was automatically generated. |
|
Thank you @tomicooler for working on this, and updated to use the deprecation system of the config class. LGTM+1. |
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute